-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrates to use JepConfig stdout, stderr from setStream method #36
Conversation
I have made an attempt to remove the setStreams method and using JepConfig's std out, err . We can also look to adding a wrapper around PrintWriter objects rather than using WriterOutputStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ooprathamm! Looks good so far - I left a couple of comments to address.
config.redirectStdout(new WriterOutputStream(out, "UTF-8")); | ||
config.redirectStderr(new WriterOutputStream(err, "UTF-8")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we foresee any potential encoding issues by manually specifying UTF-8
? e.g. Windows v Linux v Mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood! Is there any way for us to determine the encoding at runtime? e.g. is there a method we can call on out
or err
to get the encoding used and pass it to WriterOutputStream
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think this looks good. Can you please include a screenshot of the unit tests passing in your environment w/ these changes? You can run this script from within Ghidra to kick off the tests.
Can you run our example script in your environment? I'm not seeing |
|
There seems to be something wrong with Ghidrathon. I will look into it. |
@mike-hunhoff Pardon for the late response, I got occupied by exams. |
Were you able to isolate that changes introduced in this branch are the cause? Or do you believe something else may be causing the issue? |
yes, the changes are the cause. |
@mike-hunhoff the broken interpreter window has been fixed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great @ooprathamm , thank you!
Fixes #25